-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add introspection implementation #869
Add introspection implementation #869
Conversation
according to RFC 7662 the introspection mechanism is implemented
Thanks for your work on this @fetzi! I am not yet up to speed with RFC7662, so will need a bit more time to digest that thoroughly before I can make a full review of this PR. But on first pass, this looks like a good start. I have already spotted a couple of things that I think would need to be addressed to make this solution more complete and ready for release though: As you're primarily reading the data stored in the token, we're missing the opportunity to do some more interesting introspection directly from the repository, which would give us the ability to include Also, it's limited just to access tokens at the moment, but the RFC allows for introspection on all token types as well as allowing the client to hint at the token type (via the We should also think about documentation, as this is a useful feature that I'm sure many implementors will want to deploy and it requires some extra considerations. I'm also marking this for Discussion because I think that there's a really interesting twist on this that's not really explored in the RFC: why not use the Token being inspected itself as the Bearer token of the request instead of it being in the body of the request? The authorization server will then be able to tell whether or not the token is valid using already-implemented methods. This also solves a potential issue I see with this RFC. The ability/desire to allow for en masse introspection of many tokens is a bit of a weak link, which the authors sort of acknowledge. If the credentials of any client that has the ability to do this introspection is compromised, it would be open season on finding active tokens, potentially even across many clients if not implemented securely - a nasty single point of failure. I think forcing clients to authenticate with the token they're attempting to inspect goes some way to deflecting this. |
Thanks for your feedback @simonhamp. I tried to avoid breaking changes in the introspection implementation (for a timely release 😉). But yes additional data like the username would be great to be included. What else are you thinking of? The usage of the According to your security concerns: the RFC says that you should protect your introspection endpoint in some way except if its only an internal resource. Therefore I would leave the implementation of a security mechanism (oAuth2 Access Token, HTTP Basic Auth, ...) up to the implementor. This is a very important and sensible point that should be highlighted in the introspection documentation as well. The usage of the acutal token to introspect is not allowed because the RFC states in section 2.1 that the introspection request is a HTTP POST request that sends the input parameters as And yes when the feature is completed I will send an additional PR for a detailed documentation update. What do you think about the points mentioned? Maybe we should split the points up and define them in detail what should be included in the auth server and how? |
I would hold fire until we've had chance to discuss this a bit further.
I agree. This just needs to be well covered in documentation - we'd like to help people avoid building naïve implementations that are open to abuse, if possible.
I fully appreciate this point, but the fact is this is not a ratified standard as yet, 'only' a "Proposed Standard" RFC (like most of the OAuth2 RFCs) and so it's an issue that is still open for discussion among all of us who will be using/implementing against this standard in the coming years, which is especially likely if it is bundled into packages such as this. My point here is for us to engage critical thinking, not simply to implement any and every related RFC that pops up verbatim. Standards should tend to follow implementations and accepted best practice, not the other way around. So we are kind of in the driving seat on stuff like this. This is just my opinion though, weakly held. Happy to see it implemented in whatever way suits and works and ultimately is the safest :) I'll pick this up with the other maintainers (in case they haven't already seen this) and hopefully we'll get some discussion going on these points. |
Closing some loops: |
@simonhamp any update on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added some comments for this when it was submitted but forgot to finalise the review so apologies for this. I hope these comments are of use
try { | ||
$token = $this->parser->parse($jwt); | ||
|
||
$this->verifyToken($token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should use exceptions to control the flow of the program here. It is expected that we sometimes will fail these validation checks so I think this would be better handled with boolean checks
* | ||
* @throws OAuthServerException | ||
*/ | ||
private function verifyToken(Token $token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use existing verifiers for this within the package?
* | ||
* @return ResponseInterface | ||
*/ | ||
public function respondToIntrospectionRequest(ServerRequestInterface $request, ResponseInterface $response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check that the request is a POST request as per the RFC, section 2.1
Closing this for now, implementation done by @StevePorter92 in #925. |
according to RFC 7662 the introspection mechanism is implemented.